fix: preserve purged Frequencies state - #191
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes state loss in FrequentItemsSketch when a purge removes all active counters but the sketch still has non-zero stream history/error state. It does so by introducing an internal “virgin sketch” predicate and using that for serialization and merge fast paths, while keeping the public is_empty() semantics unchanged (active-items based).
Changes:
- Add an internal
is_virgin()check and use it to decide when to emit the short “empty” serialization and when to skip merges. - Update serialization to preserve
stream_weightandoffseteven whenactive_items == 0(by emitting the non-empty preamble withactive_items == 0). - Add regression tests covering round-trip serialization stability and merge preservation for the purged-to-zero-active state.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| datasketches/src/frequencies/sketch.rs | Introduces is_virgin() and uses it for merge/serialization empty fast paths to preserve purged sketch state. |
| datasketches/tests/serde_tests/frequencies.rs | Strengthens serde regression to assert purged sketches with zero active items retain weight/error state and reserialize stably. |
| datasketches/tests/frequencies_test/update.rs | Adds a merge regression ensuring purged-zero-active sketches still contribute stream/error state. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
What kind of tests did you run? There are valid cases were the table would be left empty after a purge. For example, if the table size is 1000 and all the updates are the same value. Don't confuse this sketch with the common DB "Top10" query, which can be very misleading, because in a stream of equal updates, there is no top 10! For a large stream of equal values and only a few very large values, this sketch evaluates the stream approximately and will likely report 0 most frequent values. Because the number of large value isn't large enough to statistically alter the fact that the stream is essentially flat. I think your question is more subtle. If the table is empty, and yet there is some residual history that is not in the table, should the sketch report isEmpty()= true? Well, the table is empty and the sketch will likely report 0 most-frequent values. So at that point in time, it is effectively empty. Having some residual history would allow you to continue updating the sketch from the stream, so you can query it later on. Empty in this case is not the same as a virgin sketch. It is just saying that it has no samples to statistically compute anything from at this point in time. Now, perhaps you found a bug and we should discuss it more. But I'd like more information about the exact tests you ran -- and perhaps, if I have time, I will try and repeat them and dig a litter deeper. Nonetheless, thank you for your interest and work on this! It is greatly appreciated! Cheers, |
Summary
Preserve Frequencies stream and error state when a purge removes every active counter.
The public
is_empty()method retains its existing active-item semantics. A new internalis_initial_state()check is used for serialization and merge, so a zero-active sketch with stream history is no longer treated as an unused sketch.The check requires
stream_weight,offset, and the active-item count all to be zero, avoiding state loss when a wrapped weight or an accepted inconsistent image retains other state.Such a sketch uses the existing non-empty preamble with
active_items == 0, preservingstream_weightandoffsetwithout introducing a new wire format.Closes #188.
Regression coverage
The tests deterministically produce a purged sketch with:
They verify that serialization round trips and merges preserve all of those observations, that reserialization is stable, and that a newly created or reset sketch still uses the eight-byte empty representation.
Additional regression coverage sets a serialized stream weight to zero while retaining either active items or offset, and verifies that serialization and merge preserve the remaining state.
Relationship to other implementations
Current Java, C++, and Go share the previous behavior: they define empty from the active-item count, use it to select the short empty serialization, and skip the sketch during merge.
This is therefore a shared reference-family state-loss issue, not a Rust-only format mismatch. Existing Java, C++, and Go deserializers accept a non-empty preamble with zero active items and retain its stream weight and offset on read. However, they still consider that result empty and may collapse it again during their own reserialization or merge. This PR fixes Rust round trips and Rust merges while documenting that remaining cross-language limitation.
Validation
cargo x prepare-testdatacargo x checkcargo x testcargo x lint